-
Notifications
You must be signed in to change notification settings - Fork 13.5k
tests: Fix duplicated-path-in-error fail with musl #142301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
tests: Fix duplicated-path-in-error fail with musl #142301
Conversation
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
This comment has been minimized.
This comment has been minimized.
2f57322
to
20d33c5
Compare
@@ -1,5 +1,6 @@ | |||
//@ only-linux | |||
//@ compile-flags: -Zcodegen-backend=/non-existing-one.so | |||
//@ normalize-stderr: "Error loading shared library .+:" -> "cannot open shared object file:" | |||
|
|||
// This test ensures that the error of the "not found dylib" doesn't duplicate | |||
// the path of the dylib. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then on the ERROR down here we will prefix it. the current one will be //[gnu]
and the new one will be //[musl]
, containing the messages we are expecting, so like this:
// the path of the dylib. | |
// the path of the dylib. | |
// other stuff I can't comment on because GitHub reviews aren't great | |
//[gnu]~? ERROR couldn't load codegen backend /non-existing-one.so | |
//[musl]~? ERROR idk, you're the one making the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for the hints! I think I got it working as intended now (edit: woops forgot one change, sec)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still hasn't been resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still hasn't been resolved
How exactly? It is resolved AFAICT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, my bad, prefix match 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a case to be made to make the entire test only-linux && only-gnu
since, while it is testing the error output with a nonexistent backend, the description is rather that it tests for the path not to be duplicated, which it is on musl...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original test introduced in #121978 was an anti-regression test for glibc code path's duplicate path error message, so I'm fine with limiting it to gnu-only. But this is fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I do that instead then or leave it as it is now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hm. That original issue duplicated the path in the part that isn't the dlopen error, so I guess it should be fine to keep it as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the current version is fine.
20d33c5
to
21d3790
Compare
musl's dlopen returns a different error than glibc, which contains the name of the file. This would cause the test to fail, since the filename would appear twice in the output (once in the error from rustc, once in the error message from musl). Split the expected test outputs for the different libc implementations. Signed-off-by: Jens Reidel <adrian@travitia.xyz>
21d3790
to
2988b2c
Compare
Some changes occurred in src/tools/compiletest cc @jieyouxu |
Hi @workingjubilee @wesleywiser, would you mind taking another look at this PR? :) |
@bors r=workingjubilee,fmease rollup |
Sorry, can you please leave a test comment re. why we're revisioning in the first place? It's not super obvious on a cursory glance. Also r=me after comment nit. |
Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@rustbot ready |
@bors r=workingjubilee,fmease,jieyouxu rollup |
…-musl, r=workingjubilee,fmease,jieyouxu tests: Fix duplicated-path-in-error fail with musl musl's dlopen returns a different error than glibc, which contains the name of the file. This would cause the test to fail, since the filename would appear twice in the output (once in the error from rustc, once in the error message from musl). Split the expected test outputs for the different libc implementations. Fixes rust-lang#128474
Rollup of 10 pull requests Successful merges: - #142301 (tests: Fix duplicated-path-in-error fail with musl) - #143403 (Port several trait/coherence-related attributes the new attribute system) - #143633 (fix: correct assertion to check for 'noinline' attribute presence before removal) - #143647 (Clarify and expand documentation for std::sys_common dependency structure) - #143716 (compiler: doc/comment some codegen-for-functions interfaces) - #143747 (Add target maintainer information for aarch64-unknown-linux-musl) - #143759 (Fix typos in function names in the `target_feature` test) - #143767 (Bump `src/tools/x` to Edition 2024 and some cleanups) - #143769 (Remove support for SwitchInt edge effects in backward dataflow) - #143770 (build-helper: clippy fixes) r? `@ghost` `@rustbot` modify labels: rollup
That's a tricky one. The dlopen error comes from the -gnu host trying to compile for the -musl target. Do we have a way to limit the tests based on the flavor of the host, not the target? |
Huh, odd. I don't believe so. |
musl's dlopen returns a different error than glibc, which contains the name of the file. This would cause the test to fail, since the filename would appear twice in the output (once in the error from rustc, once in the error message from musl). Split the expected test outputs for the different libc implementations.
Fixes #128474